Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Specify grid model when creating scenario #337

Closed
wants to merge 3 commits into from
Closed

Specify grid model when creating scenario #337

wants to merge 3 commits into from

Conversation

rouille
Copy link
Collaborator

@rouille rouille commented Nov 18, 2020 •
edited
Loading

Purpose

Set grid model during the building phase of a scenario (#84)

What the code is doing

A set_grid_model method in the Create class allows to set the grid model. If not called the grid_model key of the scenario_info dictionary is set to usa_tamu. When the create scenario method of the Create class is called the grid_model field (located between engine and runtime) in the *ScenarioList file will be set.

Testing

We will create/launch/extract a dummy scenario

Where to look

  • The powersimdata.scenario.create module where the set_grid_model method can be called and the scenario_info dictionary is filled.
  • The powersimdata.scenario.execute module where the Grid class is instantiated using the grid_model key of the scenario_info dictionary.

Time estimate

15min. It mirrors what we did to set the engine

Note

We will have before testing to add a new grid_model column to the *ScenarioList file and set usa_tamu as value for all existing scenario

@rouille rouille added new feature Feature that is currently in progress. grid labels Nov 18, 2020
@rouille rouille added this to the Hit Refresh milestone Nov 18, 2020
@rouille rouille mentioned this pull request Nov 18, 2020
Merged
BainanXia
BainanXia approved these changes Nov 18, 2020
View reviewed changes
Copy link
Collaborator

@BainanXia BainanXia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is clean.

@danielolsen
Copy link
Contributor

While we're updating _Builder can we also set the default interval to be "24H" rather than "144H"?

@rouille
Copy link
Collaborator Author

rouille commented Nov 19, 2020

I had to do further modifications after realizing that the source parameter of the Grid class was needed in the Builder class. Let me know what you think of the changes and if it can improved. It is kind of ugly.

@danielolsen
Copy link
Contributor

It is kind of ugly.

You are right. But I think it will work. I won't have time to think about refactoring until next week.

@rouille
Copy link
Collaborator Author

rouille commented Nov 19, 2020

It is kind of ugly.

You are right. But I think it will work. I won't have time to think about refactoring until next week.

There is no rush, there is only one grid as of now. We can think about it for some time. Perhaps @jon-hagg can help us finding a better design in the coming weeks.

@BainanXia
Copy link
Collaborator

It is kind of ugly.

You are right. But I think it will work. I won't have time to think about refactoring until next week.

There is no rush, there is only one grid as of now. We can think about it for some time. Perhaps @jon-hagg can help us finding a better design in the coming weeks.

I would suggest making the current builders for concrete classes (Texas, Western, etc) available for usa_tamu model only, since other models in the future may not have the same split as the current model does. Maybe we could have three layer of builder classes: 1) Abstract builder 2) Model builder inherit the abstract builder 3) Sub-network builder inherit model builder if any.

@danielolsen
Copy link
Contributor

Do we even really need different Builder sub-classes for the different interconnects? All object behave identically, with the only difference being their name and interconnect attributes (or name, grid_model, and interconnect after this update).

@BainanXia
Copy link
Collaborator

Do we even really need different Builder sub-classes for the different interconnects? All object behave identically, with the only difference being their name and interconnect attributes (or name, grid_model, and interconnect after this update).

It's been a while. I forgot the reason how we came up with the current design. @rouille must have a better idea on this.

@kasparm kasparm modified the milestones: Hit Refresh, Turkey Time Nov 24, 2020
@rouille rouille force-pushed the ben/model branch 3 times, most recently from 8086a00 to 31cfb0b Compare December 7, 2020 21:29
@kasparm kasparm removed this from the Turkey Time milestone Dec 9, 2020
@rouille
Copy link
Collaborator Author

rouille commented Jan 25, 2021

I close this since we will tackle this in #344

@rouille rouille closed this Jan 25, 2021
@rouille rouille deleted the ben/model branch January 25, 2021 23:08
@ahurli ahurli mentioned this pull request Mar 11, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danielolsen danielolsen danielolsen left review comments

@jenhagg jenhagg jenhagg left review comments

@BainanXia BainanXia BainanXia approved these changes

@ahurli ahurli Awaiting requested review from ahurli

Assignees

@danielolsen danielolsen

@rouille rouille

Labels
new feature Feature that is currently in progress.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@rouille @danielolsen @BainanXia @jenhagg @kasparm